Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: don't allow files to be nested inside other files #1052

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Kenny4297
Copy link

@Kenny4297 Kenny4297 commented Aug 4, 2024

Resolves #1049

Fix issue with nested path write in Volume and add tests

Issue

The Volume class had an issue where files could incorrectly be nested inside other files. It was possible to read/write files nested inside other files which should not be allowed.

Solution

To resolve this issue, I updated the writeFileSync and readFileSync methods in the Volume class to check if the parent path of the file being written to is a file. If it is, we throw an ENOTDIR error. Additionally, we check if a path being read is a directory and throw an EISDIR error if so.

Changes Made

Updated writeFileSync in volume.ts:

writeFileSync(id: TFileId, data: TData, options?: opts.IWriteFileOptions): void {
    const opts = getWriteFileOptions(options);
    const flagsNum = flagsToNumber(opts.flag);
    const modeNum = modeToNumber(opts.mode);
    const buf = dataToBuffer(data, opts.encoding);

    // Check if the parent path is a file, and throw ENOTDIR error if so
    if (typeof id === 'string') {
        const parentPath = pathModule.dirname(id);
        if (this.existsSync(parentPath) && !this.statSync(parentPath).isDirectory()) {
            throw createError('ENOTDIR', 'writeFile', id);
        }
    }

    this.writeFileBase(id, buf, flagsNum, modeNum);
}

Updated readFileSync in volume.ts:

readFileSync(file: TFileId, options?: opts.IReadFileOptions | string): TDataOut {
    const opts = getReadFileOptions(options);
    const flagsNum = flagsToNumber(opts.flag);

    // Check if the path is a directory, and throw EISDIR error if so
    if (typeof file === 'string') {
        if (this.existsSync(file) && this.statSync(file).isDirectory()) {
            throw createError('EISDIR', 'readFile', file);
        }
    }

    return this.readFileBase(file, flagsNum, opts.encoding as BufferEncoding);
}

Added Tests

I added a new test file volume-issue.test.ts under src/tests/volume/ to cover these cases. They all pass:

import { Volume } from '../../volume';

describe('Volume issue tests', () => {
    let vol: Volume;

    beforeEach(() => {
        vol = new Volume();
    });

    test('should throw ENOTDIR error when writing to a nested path within a file', () => {
        vol.writeFileSync('/foo', 'hello');

        expect(() => {
            vol.writeFileSync('/foo/bar', 'world');
        }).toThrow(/ENOTDIR/);
    });

    test('should throw EISDIR error when trying to read a directory as a file', () => {
        vol.mkdirSync('/foo');
        
        expect(() => {
            vol.readFileSync('/foo');
        }).toThrow(/EISDIR/);
    });

    test('should read file contents correctly', () => {
        vol.writeFileSync('/foo', 'hello');
        const content = vol.readFileSync('/foo', 'utf8');
        expect(content).toBe('hello');
    });

    test('should throw ENOTDIR error when reading directory contents of a file', () => {
        vol.writeFileSync('/foo', 'hello');

        expect(() => {
            vol.readdirSync('/foo');
        }).toThrow(/ENOTDIR/);
    });
});

Thank you for allowing me to contribute to this project. I learned a lot, and if there is anything else that you need me to change, please let me know!

@G-Rath G-Rath changed the title Fix issue-#1049 fix: don't allow files to be nested inside other files Aug 14, 2024
Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for the delay, for some reason I didn't see the notification for this!

it looks like a good start, but:

  • you've modified some files that shouldn't need modifying (package.json, yarn.lock, tsconfig.json)
  • you've not committed any tests

Could you please address these points, and then I'll review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Files can incorrectly be nested inside other files
2 participants